test(e2e): run the e2e host agent under systemd, not bare docker exec - #237

(Claude, prompted by Sebastian)

Honest take: this PR is only partially worth its cost as submitted — it mixes genuinely necessary new coverage with redundant and unfinished parts.

Worth keeping: the core premise is correct — docker exec alone can't validate the self-upgrade mechanism's os.Exit(0) → systemd-relaunch step, and installSystemdAgentUnit + the rollout spec (agent_upgrade_rollout_test.go) is the first test that drives that through the real, already-merged ByoHostAgentUpgrade controller + agent dpkg -i path rather than fakes (agent/reconciler/agent_upgrade_test.go only asserts the command string and a fake Exit call, never a real relaunch).

Not worth it as-is:

1. agent_upgrade_harness_test.go's [AgentUpgradeHarness] spec (~101 lines) is redundant. It kill -9s the agent directly — production never does that, it calls os.Exit(0) after a successful install — and only proves systemd's Restart=always fires, which the rollout spec already proves as a side effect (PID-change assertions at lines 93-100/136-144). It tests the harness's own systemd-unit setup, not production behavior. Suggest cutting it.

2. postinstFails (agent_upgrade_fixture_helper_test.go:52) is dead scaffolding — added "for the halt-on-explicit-failure scenario (ADR §5.3's second spec)" but only ever called with false. Neither ADR §5.3 scenario 2 (halt-on-failure) nor scenario 3 (pause-on-unrelated-unavailability) is implemented in this PR. Either write that spec or drop the parameter — right now it's an unused knob for a test that doesn't exist yet.

3. spinUpByoHostsWithSystemdAgent duplicates spinUpByoHosts almost verbatim (naming, ByoHostRunner construction, CommandArgs) instead of factoring out the shared setup. The comment at lines 73-76 acknowledges it wastes SetupByoDockerHost's /agent copy rather than forking cleanly — worth a small refactor so there's one host-spinup helper parameterized by post-setup action (exec vs. systemd-unit install), not two copies.

4. New flakiness surface is non-trivial for what it proves: hand-rolled dpkg-deb --build (nothing else in the repo builds a .deb at test time), two extra real Go binary builds via gexec, an imgpkg push, manual stdcopy demuxing — all inside Docker-in-Docker — for one happy-path scenario plus one redundant one.

Net: keep the rollout spec + systemd installer, cut the harness spec, resolve postinstFails one way or the other, and de-duplicate the spinup helper. That'd take this from ~670 new lines to something closer to the ~250 that actually earn their place.

(Claude, prompted by Sebastian)

Follow-up to my comment above — I left out something relevant to the actual reason this review got triggered: the overarching goal of making this repo independent of Platform9 by dropping the pf9- prefix from the Debian/RPM packaging (fpm package name, systemd unit name, binary filename, config/log paths, byohctl constants).

This PR doesn't advance that goal, and it goes slightly the other direction: it hardcodes several more pf9- literals into new test code — pf9-byohost-agent.service, /binary/pf9-byoh-hostagent, /etc/pf9-byohost-agent.service.d/pf9-byohost-agent.conf, /var/log/pf9/byoh, pf9-byohost-agent-fixture (all in agent_upgrade_systemd_helper_test.go / agent_upgrade_fixture_helper_test.go). These mirror production paths so they're not wrong to add as-is, but they're more places that will need to change together whenever the rename happens.

Separately, worth noting for that rename effort specifically: every upgrade this PR exercises is a version bump of the identical package identity (old/new binaries both install as pf9-byohost-agent-fixture under the same systemd unit name). The self-upgrade mechanism itself (ADR §2.1-2.2) only ever runs dpkg -i/rpm -Uvh on the pulled artifact and does nothing else — so it has no path today for a package-name change (dpkg -i of a differently-named .deb won't stop/remove the old package unless the new one declares Replaces/Breaks/Provides against the old name, the way scripts/pf9-byohost.spec's RPM Provides: lines already do for RPM but the Debian side doesn't). That's out of scope for this PR, just flagging it's a real gap for the rename work whenever that starts.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!